Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Adap-207] remove subshell around get show sql with limit predicate #249

Merged
merged 10 commits into from
Jul 3, 2024

Conversation

VersusFacit
Copy link
Contributor

@VersusFacit VersusFacit commented Jun 25, 2024

resolves #207

Problem

We want to undo the subshelling of --limit modified dbt commands, like run and show. This is particularly important for the user experience of show, where subshelling plus --limit results in non guaranteed to be ordered results for display.

Solution

Alter get_show_sql to wrap an macro dispatch call: By default we simply append a limit predicate. This leaves the door open to users who wish to alter their SQL.

Things done:

  • remove the subshell for --limit predicates on dbt show
  • add new test for the syntax error -- tested using this Redshift PR
  • all existing tests continue to work without modification

My local tests using Redshift as the adapter substrate: image

Things not done:

  • add a test case which should trigger the out of order behavior
    • I agree the usual modus operandi is to get a reprex and show we've fixed that behavior, but it's truly difficult to predict when postgres or any db will start doing this out of order. I tried valiantly with a local postgres db I put through all manner of situations; table and view, 1000 rows and 1000000 rows. Alas, nothing reliable could be ascertained. In the interest of impact, I proceeded instead to test --limit and inline limit together will flag a syntax error.

Checklist

  • I have read the contributing guide and understand what's expected of me
  • I have run this code in development, and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc.) or this PR has already received feedback and approval from Product or DX

@VersusFacit VersusFacit requested a review from a team as a code owner June 25, 2024 02:34
@VersusFacit VersusFacit self-assigned this Jun 25, 2024
@cla-bot cla-bot bot added the cla:yes label Jun 25, 2024
Copy link

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@VersusFacit VersusFacit force-pushed the ADAP-207/get_show_sql_has_no_wrapping branch 12 times, most recently from 71a34a5 to 6d7e8c0 Compare June 25, 2024 06:32
@VersusFacit VersusFacit force-pushed the ADAP-207/get_show_sql_has_no_wrapping branch from 6d7e8c0 to 780f804 Compare June 25, 2024 06:34
@VersusFacit VersusFacit changed the title Adap 207/get show sql has no wrapping [Adap-207] make get show sql have no wrapping Jun 25, 2024
@VersusFacit VersusFacit changed the title [Adap-207] make get show sql have no wrapping [Adap-207] remove subshell around get show sql with limit predicate Jun 25, 2024
@VersusFacit VersusFacit requested a review from mikealfare June 27, 2024 23:17
@mikealfare mikealfare merged commit d2842a7 into main Jul 3, 2024
15 checks passed
@mikealfare mikealfare deleted the ADAP-207/get_show_sql_has_no_wrapping branch July 3, 2024 17:43
) as model_limit_subq
limit {{ limit }}
{% macro default__get_limit_sql(sql, limit) %}
{{ compiled_code }}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VersusFacit and @mikealfare Looks like this line should be using the function parameter {{ sql }}. It works with {{ compiled_code }} I assume because of the context being set in the get_show_sql macro, but that's likely hiding a future bug.

I could be wrong, but ran into this when providing a custom version of get_show_sql in my project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change the default behavior of get_show_sql to avoid wrapping in subquery
3 participants